-
Notifications
You must be signed in to change notification settings - Fork 787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tuple and unit struct support for pyclass macro #1504
Conversation
a284b88
to
b95bccf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks for implementing this! Changes look good to me, though I have one follow-up thought around #[pyo3(get, set)]
.
Needs a CHANGELOG entry.
} | ||
_ => unreachable!(), | ||
} else { | ||
bail_spanned!(field.span() => "get/set are not supported on tuple struct field"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what the reason for disallowing get/set
is? I was thinking that it should be possible, but a name
would be required.
e.g. this seems a plausible combination to me:
#[pyclass]
struct PyFoo(
#[pyo3(get, set, name = "inner", from_py_with = "pytimestamp_to_int")] i32,
);
I'm happy for us to create an issue to leave this for a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was disallowed just to avoid proc-maco panic when user try to use get/set
on it. We can certainly add support for it, I think we can do the follow-up after #1507 merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @davidhewitt and @messense This's nice! And as a local wrapper of external structure, I think we still can't get them work with nested getter and setter, which is get_all
and set_all
?
// By default, don't allow creating instances from python. | ||
assert!(typeobj.call((), None).is_err()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Co-authored-by: Yuji Kanagawa <yuji.kngw.80s.revive@gmail.com>
Thanks! |
Fixes #287 (comment)